Parse source to output file map from aspect outputgroup JSON files #49
Conversation
There was a problem hiding this comment.
@thiagohmcruz nice one!
Flipping indexing on SGTM - added a few suggestions inline.
thiagohmcruz
force-pushed
the
thiago/source-out-map-as-input
branch
from
1ec8fd2
to
c45ad55
Compare
thiagohmcruz
force-pushed
the
thiago/source-out-map-as-input
branch
from
c45ad55
to
5685d7d
Compare
There was a problem hiding this comment.
Nice one @thiagohmcruz ! Minor tweak for pulling the config down to the request level. It might be a more nuanced change
| // so one config per line. This is intentionally simple at the moment but we can add more validation or a more reliable file format later. | ||
| // | ||
| // TODO: Codable? | ||
| // TODO: Not load from disk all the time but still detect changes in the file and refresh in-memory values? |
There was a problem hiding this comment.
Yeah I think what you've got here is good for now and this is a tricky thing to deal with.
| </plist> | ||
| """ | ||
| guard let converter = BPlistConverter(xml: xml) else { | ||
| fatalError("Failed to allocate converter") |
There was a problem hiding this comment.
I wonder if you'd also instead tried propagate an empty data blob in this case or does that hang the build service? I imagined they had some form of error handling for a message like this and we could hook that
Minor - we should have a nice formatter on the repo longer term probably 😅
| // Create key | ||
| let sourceKey = filePath.replacingOccurrences(of: workingDir, with: "").replacingOccurrences(of: (info.config.bazelWorkingDir ?? ""), with: "") | ||
| // Loops until found | ||
| for (_, json) in info.outputFileForSource { |
There was a problem hiding this comment.
I assume we can circle back if perf is an issue on this. Is it fair to say the runtime complexity of this O(N) output files for a given project?
From what I looked at in going into outputFileForSource - I was wondering if we could compute the dictionary key / source file and get this in O(1) time
| fatalError("[ERROR] Failed to build SDK path, platform is empty.") | ||
| } | ||
|
|
||
| return "\(info.xcode)/Contents/Developer/Platforms/\(msg.platform).platform/Developer/SDKs/\(msg.sdk).sdk" |
There was a problem hiding this comment.
I wondered if we can get this from the request messages in Xcode 14 - if they have SDKs living outside of Xcode.
| // Xcode will try to find the data store under DerivedData so we need to symlink it to `BazelBuildServiceConfig.indexStorePath` | ||
| // if that value was set in the config file. | ||
| // | ||
| // This function manages creating a symlink if it doesn't exist and it creates a backup with the `.default` suffix before doing that. Additionally, |
There was a problem hiding this comment.
Massive improvement and putting it here - looks overall way cleaner than what it did before
| log("[ERROR] Failed to start BEP stream with error: " + error.localizedDescription) | ||
| } | ||
| } else { | ||
| log("[WARNING] BEP string config key 'BUILD_SERVICE_BEP_PATH' empty. Bazel information won't be available during a build.") | ||
| } | ||
|
|
||
| // This information was not explicitly available in `CreateSessionRequest`, parse from `CreateBuildRequest` instead |
There was a problem hiding this comment.
Thanks for leaving this comment here - I wonder if that's because it loads the workspaces async.. Seems good like this none the less
| @@ -68,6 +68,8 @@ private var workspaceName = "" | |||
| // TODO: parsed in `IndexingInfoRequested`, there's probably a less hacky way to get this. | |||
| // Effectively `$PWD/iOSApp` | |||
| private var workingDir = "" | |||
| // Path to derived data for the current workspace | |||
| private var derivedDataPath = "" | |||
There was a problem hiding this comment.
For the next time we look at it - it help future bistandard to have comment about the global situation of the state problem of these vars!
path/to/foo.xcodeproj/xcbuildkit.configBUILD_SERVICE_INDEXING_DATA_DIRconfig), this allows fast load on Xcode launch[WARNING|INFO|ERROR]logs to help with troubleshooting.xcodeprojpath fromCreateSessionRequestfileHandle?.closeFile()is deprecated, calling.close()nowfileHandle.waitForDataInBackgroundAndNotify()is necessary to read the BEP when installed as a pkg on macOSWorkspaceInfo,IndexingService,BazelBuildServiceConfig,BEPService,WorkspaceInfoKeyable(see responsibilities of each in comments in code)